Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Windows fix: normalizes extra_incdir, universal_newlines on preproces… #4

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

per42
Copy link

@per42 per42 commented Sep 14, 2016

…sor output.

When input is a filename without directory, the dirname is empty, which inserts -I, instead of -I ., which breaks the command.
Remedied with os.path.normpath

On Windows with MinGW gcc, preprocessor output has Windows newlines, which pycparser can't handle.
Remedied by universal_newlines=True for subprocess.

Py3: universal_newlines=True changes subprocess.communicate to take and return strings instead of bytes. Adjusted for this.
Current master reads input headers as bytes. This causes code.encode to fail. Changed cli to open files as text fixes this.

Tested with test.py on Python 2.7 and 3.5 on Windows 10.

'cpp', '-nostdinc', '-D__attribute__(x)=', '-I', BUILTIN_HEADERS_DIR,
] + extra_cpp_args + ['-'], stdin=subprocess.PIPE, stdout=subprocess.PIPE)
proc = subprocess.Popen(
['cpp', '-nostdinc', '-D__attribute__(x)=',
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please avoid making unnecessary code style/spacing changes.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I exceeded pep8 max line length with the extra argument. Please suggest the style you prefer.

@tarruda
Copy link
Owner

tarruda commented Sep 15, 2016

python 3.4 test failing :(

…sor output.

When input is a filename without directory, the dirname is empty, which inserts `-I `, instead of ` -I .`, which breaks the command.
Remedied with os.path.normpath

On Windows with MinGW gcc, preprocessor output has Windows newlines, which pycparser can't handle.
Remedied by universal_newlines=True for subprocess.

Py3: universal_newlines=True changes `subprocess.communicate` to take and return strings instead of bytes. Adjusted for this.
Current master reads input headers as bytes. This causes `code.encode` to fail. Changed `cli` to open files as text fixes this.

Tested with `test.py` on Python 2.7 and 3.5 on Windows 10.
'-I', BUILTIN_HEADERS_DIR] +
extra_cpp_args + ['-'],
stdin=subprocess.PIPE, stdout=subprocess.PIPE, universal_newlines=True)
result = proc.communicate(input=code)[0]
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

communicate should be called only once. doc:

Read data from stdout and stderr, until end-of-file is reached. Wait for process to terminate.

Use universal_newlines to get platform independent source code strings. Note doc:

The type of input must be bytes or, if universal_newlines was True, a string.
communicate() returns a tuple (stdout_data, stderr_data). The data will be bytes or, if universal_newlines was True, strings.

p = AutoPxd(hdrname)
p.visit(parse(code, extra_cpp_args=['-I', extra_incdir]))
return str(p)


@click.command()
@click.argument('infile', type=click.File('rb'), default=sys.stdin)
@click.argument('outfile', type=click.File('wb'), default=sys.stdout)
@click.argument('infile', type=click.File(), default=sys.stdin)
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

'rt' is default

@tarruda
Copy link
Owner

tarruda commented Nov 4, 2017

Sorry for the big delay in reviewing/merging this. Do you mind rebasing your PR on top of current master?

Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants